feat(research): wire answerability into campaign admission - #7469
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Implementation self-review at exact head
Disposition: hold pending domain-aware approval, current hosted checks, and independent approval; no self-approval or merge performed. |
Independent review: durable artifact proof is too weakExact head reviewed: The new Independent reproduction from the exact-head review worktree used the existing example manifest, changed only the design to The result is materially weaker than the issue’s stated gap (“verify checksums” and “prove the durable path”) and than the existing artifact-catalog validator, which does regular-file, path-policy, and SHA-256 checks. Please either route the artifact surface through that public catalog/checksum contract or make Disposition: |
Exact-head self-reviewReviewed and validated the revised PR at exact head
Disposition: |
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head answerability review — PR #7469 / Issue #7090
Reviewed exact head f4bd271d510b916461c830866b5a2e1ad3807124 against current origin/main a1892cf453973cd19e7bbba158a9f4132009bcee in linked worktree /home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260818.
Implementation proof:
- Focused answerability, admission, campaign-manifest, Issue #3425, and camera-ready tests: 96 passed.
- Changed Python entry points and tests compile successfully.
- Ruff check and format check on all changed Python files: passed.
bash -n scripts/benchmark/run_issue_3425_empirical_vertical_slice.sh: passed.git diff --check origin/main...HEAD: passed.- Current-base CAS: passed with observed main/head matching the reviewed SHAs.
The implementation is aligned with the bounded #7090 contract: required proof surfaces are evaluated through the canonical admission seam; path-only durable declarations cannot promote required artifact proof; required failures/unavailable/not-run/fallback/degraded surfaces fail closed; optional unavailable surfaces remain explicit; and the camera-ready launcher stops before preflight/execution when --require-answerable is requested. The public evaluator writes no packet in its proof path.
Claim boundary is preserved: no campaign, simulation, SLURM job, benchmark outcome, planner ranking, evidence admission, publication, or dissertation result was produced. The frozen research-yield dimensions remain process diagnostics, not live empirical outcomes.
The PR body correctly marks Domain-Aware Approval as pending. Implementation integrity is accepted, but experimental-validity/launch-gate review remains a required owner decision.
gate-verdict: accepted @ f4bd271
base-policy: current-base @ f4bd271
pr-metadata: reconciled @ aa6e45f0d36608db7023700cc720b752981795e25695648a2842a837ae716fc3
merge-ready: no (Domain-Aware Approval pending; hosted checks are not terminal)
domain-approval: pending
compute: none
github-actions
Bot
added
the
review-bot-auto
🔍 PR Contract Check SummaryOverall Status: 🟢 PASSED
ℹ️ Info
This check is mechanized. Please resolve any blockers to pass CI. |
PR reconciliation — canonical consolidated ownerPRs #7092 and #7102 are now closed as superseded. This PR is the sole owner of the combined research-yield, answerability-proof, and camera-ready admission integration. Current disposition: blocked on maintainer Domain-Aware Approval for #7090. Keep this PR in draft until the decision explicitly accepts the production admission semantics. After approval, refresh onto current This consolidation does not authorize compute, evidence admission, or a research result. |
|
Merge-ordering note from the PR-resolution lane. #7102 and #7092 are landing first as the reviewed diagnostic-only halves of this change. Both now Please rebase this PR onto No action is requested here beyond the rebase; this comment records the ordering so the superset |
|
Correction to my previous comment — it was written against stale state and is wrong. #7092 and #7102 are not landing first. Both were closed as superseded by this PR at 07:33 UTC, What does carry over: both closed PRs completed exact-head review at their final heads with green
Both were reviewed as diagnostic-only: no claim-ledger, evidence-admission, or preregistration |
Independent exact-head review — PR #7469 / Issue #7090
This review does not authorize campaign execution, simulator episodes, SLURM/compute, model |
Independent exact-head review — PR #7469 / Issue #7090
The implementation is not merge-ready. The admission contract still has these actionable findings:
These are contract-level blockers, not a request to run a campaign. Fixes should remain local and
|
Independent exact-head review — PR #7469 / Issue #7090
This review records implementation proof only; it does not authorize campaign execution or |
Independent exact-head review — PR #7469 / Issue #7090
This review records implementation proof only; it does not authorize campaign execution or |
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Domain-aware ruling — REQUEST CHANGES
Reviewed exact head 3ad253f054e9ba2306780630c3f0f011a81231df against #7090, #7031, #7029, and #7340.
The launch-owner seam and authority boundary are correct in principle. Required non-passed proof fails closed; required fallback/degraded/unavailable producers fail closed; optional unavailable, failed, and not-run proof remains visible without becoming zero-valued evidence; and the launch receipt states compute_authorized=false, evidence_status=not_run, and benchmark_success=false. Therefore, the gate does not itself authorize compute, admit evidence, or establish experimental validity.
Approval is withheld because the executable proof floor is not yet claim-specific or substitution-safe.
-
Named surfaces are not bound to canonical proof kinds.
_run_surfacecheckskindagainst one global set, not against the named surface. A decision-capable manifest can therefore assign the same passingcommand/pytest_contractcheck toproducer,preregistration,evidence_contract,analysis, andartifact, self-declareproof_class: decision_capable, and obtain fivepassedsurfaces. This bypasses the producer, artifact, and diagnostic-specific guards. A generic test exit code is not producer-to-estimand-to-analysis-to-artifact proof. -
Diagnostic and fixture classification is incomplete.
_run_result_packetaccepts any schema-valid #7029 packet and does not inspect its controlled evidence tier or admission state. A validsmoke_diagnostic/diagnostic_onlypacket can therefore be recorded as passed proof for a decision-capable campaign._run_artifact_catalogrelies on free-text substrings inclaim_boundaryand does not bindcatalog_id, selectedartifact_idvalues, or their file digests to the manifest's declared artifact plan. This permits a valid but unrelated or neutrally worded diagnostic/fixture catalog to satisfy artifact proof, while the substring rule can also reject an eligible catalog merely because its boundary negates the word “diagnostic.” -
The binding can name bytes other than those evaluated or executed. The manifest is parsed and later re-read for hashing; the camera configuration is loaded before the gate hashes it; file-backed proof is hashed before its validator re-reads it. A concurrent mutation can therefore produce a receipt whose digest does not identify the bytes used for evaluation or execution. Re-evaluation between preflight and run does not close this intra-invocation gap.
Exact minimal fixes
- Add an explicit surface-to-kind/validator map. For required decision-capable proof, permit only the canonical validator for that surface. Do not allow one generic
pytest_contractresult to substitute across surfaces. Retainmanifest_rowsas diagnostic-only anddurable_pathas unavailable. - Add exact semantic identity checks. Bind the campaign, question, estimand, producer receipt, preregistration/evidence-contract identity, analysis identity, artifact
catalog_idplus selectedartifact_id/digest set, and any result-packet identity. Rejecttests/fixtures/**and controlled diagnostic/fixture tiers for decision-capable proof; do not use free-text substring classification as the authority. - In
_run_result_packet, inspect the validated packet object.smoke_diagnostic,visualization_fixture,diagnostic_only, andunavailable_causal_inferencemust not pass decision-capable proof. A passing packet remains launch-contract proof only; it is not research-evidence admission. - Read and hash each manifest, configuration, and proof input from the same immutable bytes, or verify an unchanged pre/post digest and fail on drift. Ensure the executed configuration is the configuration bound in the receipt.
- Preserve the approved optionality rule: an optional unavailable surface remains an explicit warning and must not block. All required missing, unavailable, failed, not-run, fallback, or degraded proof remains blocking. Align the documentation with that exact rule rather than silently making optional result-packet availability mandatory.
- Add adversarial regressions for wrong-kind substitution, one-command-for-five-surfaces, diagnostic result-packet promotion, unrelated/fixture artifact catalogs, negated diagnostic wording, and input mutation.
This ruling concerns launch-gate implementation integrity only. It does not approve the experimental design, authorize compute, establish baseline comparability, admit evidence, or support benchmark, planner, safety, dissertation, or paper claims. #7340 remains diagnostic-only and separately blocked pending a real matched campaign, canonical provenance, and domain/compute approval.
Separate merge-readiness findings
- #7489 was an unrelated current-main test-contract defect. It is closed and repaired on current
mainat98ef78ca8556f40c73519819311a698d6e5f8544; it is not a basis for this domain ruling. - Exact head
3ad253f...has its own independent hosted-CI blocker:changed-coverage-gatereports 23.3% changed-line coverage forrobot_sf/benchmark/research_answerability.py, below the 80% threshold. This must be fixed separately.
domain-aware-verdict: request-changes @ 3ad253f054e9ba2306780630c3f0f011a81231df
compute-authorized: false
evidence-admitted: false
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #7469 / Issue #7090
Reviewed exact head e806ac27116d93c404278d9f35726295a4997286 against current origin/main
8dbd696b3bd8f903298c964e49842dac3ebc1e0 in linked worktree
/home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260818.
The domain ruling's implementation findings are addressed:
- decision-capable surfaces now require their canonical proof kind/validator;
- strict identities bind campaign, question, estimand, receipts, artifact catalog selections and
digest sets, and result-packet identity; - required fixture/diagnostic catalogs and packets are rejected by path/source classification and
evidence tier/admission state rather than claim-boundary substring heuristics; - stable pre/post digest checks fail closed on input mutation, including the camera configuration
bound by the launcher; - optional unavailable proof remains a warning, while required non-passed proof remains blocking.
Proof:
- focused suite: 102 passed;
- existing radius-binding compatibility regression: 1 passed;
- full local
pr_ready_check.shagainst current main: passed for core/optional tests, changed-file
gates, ratchets, freshness, and base drift; - changed executable-line coverage is 88.4% (76/86), above the 80% minimum; the 100% goal is
reported as a warning; - no compute, benchmark campaign, evidence admission, or scientific claim was performed.
Implementation integrity is accepted at this head. Domain-aware approval for launch-gate semantics
and terminal hosted checks remain pending; this is not merge-ready.
gate-verdict: accepted @ e806ac27116d93c404278d9f35726295a4997286
base-policy: current-base @ 8dbd696b3bd8f903298c964e49842dac3ebc1e0
pr-metadata: reconciled @ e42cf9a7e45ffc95ae588fbe34781771851e521cad9b7e411060dc6501bb738e
merge-ready: no (Domain-Aware Approval and terminal hosted checks pending)
domain-approval: pending
compute: none
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #7469 / Issue #7090
Reviewed exact head 6c821dee13b993e0d36d4f4f54cfdfc997b55438 against current origin/main
c8ad73fbe3bca3f142709687f535056595923aa8 in linked worktree
/home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260818.
The domain ruling's implementation findings are addressed:
- decision-capable surfaces now require their canonical proof kind/validator;
- strict identities bind campaign, question, estimand, receipts, artifact catalog selections and
digest sets, and result-packet identity; - required fixture/diagnostic catalogs and packets are rejected by path/source classification and
evidence tier/admission state rather than claim-boundary substring heuristics; - stable pre/post digest checks fail closed on input mutation, including the camera configuration
bound by the launcher; - optional unavailable proof remains a warning, while required non-passed proof remains blocking.
Proof:
- focused suite: 108 passed;
- existing radius-binding compatibility regression: 1 passed;
- body-independent
pr_ready_check.shin interim mode against current main: passed for core/optional
tests, changed-file gates, ratchets, freshness, and base drift with a clean tracked tree; - changed executable-line coverage is 88.4% (76/86), above the 80% minimum; the 100% goal is
reported as a warning; - no compute, benchmark campaign, evidence admission, or scientific claim was performed.
Implementation integrity is accepted at this head. Domain-aware approval for launch-gate semantics
and terminal hosted checks remain pending; this is not merge-ready.
gate-verdict: accepted @ 6c821dee13b993e0d36d4f4f54cfdfc997b55438
base-policy: current-base @ c8ad73fbe3bca3f142709687f535056595923aa8
pr-metadata: reconciled @ ced4a1fe99af88ad1043531ff20a6e8fd6076e212292d8ac88a92ab9348f5921
merge-ready: no (Domain-Aware Approval and terminal hosted checks pending)
domain-approval: pending
compute: none
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #7469 / Issue #7090
Reviewed exact head d4374e9576337982c9f691a5ac141f52d9044628 against current origin/main
c8ad73fbe3bca3f142709687f535056595923aa8 in linked worktree
/home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260818.
The domain ruling's implementation findings are addressed:
- decision-capable surfaces now require their canonical proof kind/validator;
- strict identities bind campaign, question, estimand, receipts, artifact catalog selections and
digest sets, and result-packet identity; - required fixture/diagnostic catalogs and packets are rejected by path/source classification and
evidence tier/admission state rather than claim-boundary substring heuristics; - stable pre/post digest checks fail closed on input mutation, including the camera configuration
bound by the launcher; - optional unavailable proof remains a warning, while required non-passed proof remains blocking.
Proof:
- focused suite: 108 passed;
- existing radius-binding compatibility regression: 1 passed;
- body-independent
pr_ready_check.shin interim mode against current main: passed for core/optional
tests, changed-file gates, ratchets, freshness, and base drift with a clean tracked tree; - changed executable-line coverage is 88.4% (76/86), above the 80% minimum; the 100% goal is
reported as a warning; - no compute, benchmark campaign, evidence admission, or scientific claim was performed.
Implementation integrity is accepted at this head. The deterministic answerability contract tests
are included in pull-request fast shards so exact-head changed coverage exercises the implementation.
Domain-aware approval for launch-gate semantics
and terminal hosted checks remain pending; this is not merge-ready.
gate-verdict: accepted @ d4374e9576337982c9f691a5ac141f52d9044628
base-policy: current-base @ c8ad73fbe3bca3f142709687f535056595923aa8
pr-metadata: reconciled @ 3ef63b00e3c3f798a117dc010ef0ccccbb325fda8c8592757af0c19dcba6d02d
merge-ready: no (Domain-Aware Approval and terminal hosted checks pending)
domain-approval: pending
compute: none
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #7469 / Issue #7090
Reviewed exact head d4374e9576337982c9f691a5ac141f52d9044628 against current origin/main
c8ad73fbe3bca3f142709687f535056595923aa8 in linked worktree
/home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260818.
The domain ruling's implementation findings are addressed:
- decision-capable surfaces now require their canonical proof kind/validator;
- strict identities bind campaign, question, estimand, receipts, artifact catalog selections and
digest sets, and result-packet identity; - required fixture/diagnostic catalogs and packets are rejected by path/source classification and
evidence tier/admission state rather than claim-boundary substring heuristics; - stable pre/post digest checks fail closed on input mutation, including the camera configuration
bound by the launcher; - optional unavailable proof remains a warning, while required non-passed proof remains blocking.
Proof:
- focused suite: 108 passed;
- existing radius-binding compatibility regression: 1 passed;
- body-independent
pr_ready_check.shin interim mode against current main: passed for core/optional
tests, changed-file gates, ratchets, freshness, and base drift with a clean tracked tree; - changed executable-line coverage is 88.4% (76/86), above the 80% minimum; the 100% goal is
reported as a warning; - no compute, benchmark campaign, evidence admission, or scientific claim was performed.
Implementation integrity is accepted at this head. The deterministic answerability contract tests
are included in pull-request fast shards so exact-head changed coverage exercises the implementation.
Domain-aware approval for launch-gate semantics
and terminal hosted checks remain pending; this is not merge-ready.
gate-verdict: accepted @ d4374e9576337982c9f691a5ac141f52d9044628
base-policy: current-base @ c8ad73fbe3bca3f142709687f535056595923aa8
pr-metadata: reconciled @ 8f1f28be54c440dbd012095873ac73707e4cee71b4931e264cd724eaf8ec0c96
merge-ready: no (Domain-Aware Approval and terminal hosted checks pending)
domain-approval: pending
compute: none
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head research workflow review
Reviewed PR #7469 at refreshed head d2f836be7dad82ed9385b5cc14c74d845d92ac45 against current
main base c71543025108710791b37cafea35b9c8fe3b391e.
Local proof
- Current-main refresh required one
tests/conftest.pyconflict repair; the answerability fast-lane
entries and current-main scenario smoke entries were both preserved. - Answerability, preflight, manifest-runner, #3425 wrapper, and camera-ready tests: 126 passed.
- Ruff check/format, Python compilation, shell syntax, and
git diff --checkpassed for the
changed scope. - No campaign, simulator episode, SLURM submission, compute action, evidence promotion, or
publication was run.
Research disposition
This is launch-gate contract and blocker-resolution infrastructure, not a planner or benchmark
result. Required non-passed proof remains fail-closed; fallback, degraded, unavailable, failed,
and not-run states cannot authorize admission. Domain review must decide whether the answerability
semantics are appropriate before this research-facing gate can merge.
Admission state
Hosted CI currently reports 2 successes, 1 expected skip, and 27 pending checks with no failures.
The PR remains draft and independent domain-aware approval is pending; no merge-ready admission is
appropriate.
base-policy: ordinary-cas @ d2f836b
pr-metadata: reconciled @ 9334243423c36a920033441fafd4c1a2f0b4042aa382eb36ab147276a6445979
gate-verdict: blocked @ d2f836b
ed4dbe0 to
80e35d7
Compare
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head research-admission review
Reviewed owner head 80e35d7c8721380d0573d688561c128a1f9d8e71 against current origin/main 39eb9f54f9f1637253cc8b598f134977e6860687.
- Focused answerability, preflight, manifest, camera-ready, and #3425 contract suite: 112 passed.
- Required-answerability smoke: exit 2 with
state=not_declaredbecause the fixture manifest does not declareanswerability.v1; no summary packet, campaign, compute, or evidence admission was produced. - Docs/evidence integrity: 17 changed files and the full 2,006-file Markdown scan passed; proof consistency returned no findings.
- Fast-lane routing: one registered fast contract with zero findings; evidence-registry ratchet remained 421 findings against a 421 baseline with no net-new findings.
- Final readiness: passed core and optional lanes with a clean tree; stamp
output/validation/pr_ready/autopilot-pr-7469-readiness-39eb-r1-20260821.json. - Changed coverage:
research_answerability.py88.9% (80/90), above the 80% gate; the 100% target remains warning-only.
The implementation is fail-closed around decision-capable answerability, stable manifest/config/receipt identities, artifact/result provenance, fallback/degraded exclusions, and repository-root confinement. The proof establishes implementation and launch-gate integrity only; no benchmark, planner, model, metric, safety, transfer, publication, dissertation, or paper-facing result is admitted.
The PR remains blocked by draft/ruled state, pending independent approval for broader evidence admission, dependency/parent direction, and terminal hosted checks. The bounded domain approval in the body is not a waiver for those broader gates; no merge-ready label or merge waiver is inferred.
base-policy: ordinary-cas @ 80e35d7
pr-metadata: reconciled @ db7f5f0192faeb23868ba6adf392c5115f7adb2df66dbc76333da82d0f41e6ee
gate-verdict: blocked @ 80e35d7
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Review: blocked at the current exact head
I reviewed PR #7469 at exact owner head 80e35d7c8721380d0573d688561c128a1f9d8e71 against
origin/main 39eb9f54f9f1637253cc8b598f134977e6860687.
The implementation-contract proof is coherent and fail-closed:
- The focused answerability, preflight, manifest, camera-ready, and #3425 suite passed 112 tests.
- The real #3425 required-answerability smoke exited 2 with
state=not_declared; no campaign,
summary packet, evidence artifact, or compute action was produced. - Docs/evidence integrity, proof consistency, fast-lane routing, and the 421-finding evidence
ratchet passed with zero net-new findings. - Final PR readiness passed core and optional lanes at the exact base/head. The clean stamp is
output/validation/pr_ready/autopilot-pr-7469-readiness-39eb-r1-20260821.json; changed-line
coverage is 88.9% (80/90), above the enforced 80% minimum, with the 100% target advisory.
This is implementation and launch-gate integrity evidence only. It establishes no benchmark,
planner, model, metric, safety, transfer, publication, dissertation, or paper result. The PR is
correctly kept draft and blocked: independent approval for broader evidence admission, parent or
dependency direction, and terminal hosted checks remain unresolved. The bounded domain approval is
not a waiver for those blockers. I did not add merge-ready or merge the PR.
base-policy: ordinary-cas @ 80e35d7
pr-metadata: reconciled @ 42b8dda0deaa0ba6f294fce8e27b104f8b9a921af3495d7f3645013d8a862029
gate-verdict: blocked @ 80e35d7
|
Autopilot exact-head admission carrier for PR #7469. Implementation-contract proof passed at head base-policy: ordinary-cas @ 80e35d7 |
…0260818' into review/pr-7469-current-20260821g
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head research workflow review — PR #7469 / issue #7090
No actionable implementation correctness finding remains at exact head
d99d6a10e889df953a840e87787d15e4ca3b2e3c against immutable current origin/main
75387dd4dbbaac7f23474b6785df1327ad31dc8d.
The strict decision-capable admission correction is present and fail-closed:
analysis.dry_run_status must be passed, design.power_status must be adequate, and waived
values return named blocked states rather than answerable. Required proof remains
surface-specific and bound to exact manifest, config, receipt, catalog, and packet identities;
fixture, fallback, degraded, unavailable, failed, and not-run required proof remains non-admitting.
This is launch-gate infrastructure, not a campaign or scientific result.
Local proof
- Exact current-base answerability, preflight, manifest-runner, #3425 wrapper, camera-ready, and
manifest-contract suite: 112 passed. - Strict waived dry-run/power regressions passed with
blocked_analysis_contractand
blocked_underpowered, neveranswerable. - Fast-lane routing: 1 changed module, 1 registered observation, 0 findings.
- Docs/evidence integrity: 17 changed files passed; full Markdown scan: 2,011 files passed.
- Ruff check/format, Python compilation, Bash syntax, and
git diff --checkpassed. - Bounded #3425 smoke: the default path emitted 27
diagnostic_onlyrows; the required
RUN_CAMPAIGN=1path stopped with exit 2 andstate=not_declaredbefore campaign execution,
with no benchmark summary or compute action. - Immutable-base final readiness reached 4,809 passed and 3 skipped before stopping on two
unrelated current-main defects: the unreviewed map-runner assertion tracked by #7692 and the
evidence-registry baseline companion gap tracked by #7634. No passed clean readiness stamp is
claimed for this refreshed head. - No campaign, simulator episode, SLURM submission, compute action, model download, evidence
admission, publication, or scientific result was produced.
Domain and merge boundary
The bounded direction approval for #7090 remains separate from implementation proof. The PR is
correctly kept draft/blocked: independent domain-aware approval, dependency/parent direction, the
two current-main readiness blockers, and terminal hosted checks remain open. No merge-ready
admission is appropriate.
base-policy: immutable-current-main @ 75387dd4dbbaac7f23474b6785df1327ad31dc8d
review-claim: reviewed @ d99d6a10e889df953a840e87787d15e4ca3b2e3c
gate-verdict: blocked @ d99d6a10e889df953a840e87787d15e4ca3b2e3c; blockers #7692/#7634 and domain/dependency/hosted gates remain
merge-ready: no @ d99d6a10e889df953a840e87787d15e4ca3b2e3c
pr-metadata: reconciled @ 2837890fd50214f01fe6a55198278ebada5e0a7b8356d6fb63e4a1d692b37d2a
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Review result
No actionable implementation finding remains on the exact owner head. The repair correctly
confines repository-declared campaign configuration and output roots after symlink resolution, and
the new regression test exercises the escape case. The change remains a bounded launch-gate
implementation-contract slice; it does not establish an empirical or scientific result.
Evidence
- Exact owner head:
a257ff17562f7654d75131fbf8c6d604d36637a5. - Current base:
75387dd4dbbaac7f23474b6785df1327ad31dc8d. - Reconciled body metadata:
33034ee22eecd9b351b3a42f97a3534a91c9a7f0fc01f789c6fa20d538188d96. - Focused suite: 113 passed; benchmark namespace: 7 passed.
- Real #3425 preflight: exit 2,
research_answerability_blocked,state=not_declared, no
summary.json; no campaign, simulator episode, SLURM submission, or compute ran. - Full readiness: 5,075 passed, 3 skipped, 4 warnings, blocked only by current-main #7634 and
#7692 contract gaps; no readiness stamp issued.
The PR is not merge-ready because it is still a draft, lacks the guarded merge-ready label, has
pending broader/domain admission, and carries a blocked exact-head verdict. Those are deliberate
fail-closed statuses, not waivers.
base-policy: ordinary-cas @ 75387dd4dbbaac7f23474b6785df1327ad31dc8d
pr-metadata: reconciled @ 33034ee22eecd9b351b3a42f97a3534a91c9a7f0fc01f789c6fa20d538188d96
gate-verdict: blocked @ a257ff17562f7654d75131fbf8c6d604d36637a5
merge-ready: no @ a257ff17562f7654d75131fbf8c6d604d36637a5
compute: none
|
Admission carrier for exact owner head
Focused proof is green (113 answerability/preflight/manifest/camera-ready tests; namespace 7), |
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head research workflow review — PR #7469 / issue #7090
No actionable implementation correctness finding remains at exact remote head
a257ff17562f7654d75131fbf8c6d604d36637a5 against current origin/main
2956422ae3ff06bcd984b33692ee0b42f48f73c4.
The new path-confinement change rejects repository-relative traversal and symlink escapes for
campaign outputs, required paths, and the declared campaign configuration; its focused symlink
regression passes. The strict decision-capable admission correction is present and fail-closed:
analysis.dry_run_status must be passed, design.power_status must be adequate, and waived
values return named blocked states rather than answerable. Required proof remains surface-specific
and bound to exact manifest, config, receipt, catalog, and packet identities; fixture, fallback,
degraded, unavailable, failed, and not-run required proof remains non-admitting. This is launch-gate
infrastructure, not a campaign or scientific result.
Local proof
- Exact current-base answerability, preflight, manifest-runner, #3425 wrapper, camera-ready, and
manifest-contract suite: 113 passed. - Ruff check/format, Python compilation, Bash syntax, and
git diff --checkpassed for the focused
changed surface. - Bounded #3425 smoke with all-extras/rvo2 available: the default preflight emitted 27
diagnostic_onlyrows and completed camera-ready preflight; the requiredRUN_CAMPAIGN=1path
stopped with exit 2 andstate=not_declaredbefore campaign execution. No benchmark summary or
compute action was produced. - Hosted validation for the remote head: 31 checks passed and 2 expected checks were skipped
(coverage-gate,merge-queue-gate); no failures or pending checks. - Source-equivalent current-base readiness on local merge commit
6417dd494a66ce005eb34a444e7177c78f8f4a1breached4838 passed, 3 skippedbefore stopping on
the independent unreviewed map-runner assertion tracked by #7692 and a host-only CUDA/NVML
initialization failure. The #7634 evidence-registry companion blocker is resolved in current main;
no passed clean readiness stamp is claimed. CUDA failure is environment evidence only, not a GPU
or provider result. - No campaign, simulator episode, SLURM submission, compute action, model download, evidence
admission, publication, or scientific result was produced.
Domain and merge boundary
The bounded direction approval for #7090 remains separate from implementation proof. The PR is
correctly kept draft/blocked: independent domain-aware approval, dependency/parent direction, and
the #7692 current-main readiness blocker remain open. No merge-ready admission is appropriate.
base-policy: ordinary-cas @ 2956422ae3ff06bcd984b33692ee0b42f48f73c4
review-claim: reviewed @ a257ff17562f7654d75131fbf8c6d604d36637a5
gate-verdict: blocked @ a257ff17562f7654d75131fbf8c6d604d36637a5; blocker #7692 and domain/dependency gates remain
merge-ready: no @ a257ff17562f7654d75131fbf8c6d604d36637a5
pr-metadata: reconciled @ 30a23adec7f5b536c884dff69d4c0fa60b8bf2bbf3d3b278c2414edb01eca061
ll7
changed the title
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head research workflow review — PR #7469 / issue #7090
No actionable implementation correctness finding remains at exact remote head
a257ff17562f7654d75131fbf8c6d604d36637a5 against current origin/main
cbb7470f0d081fb257b22f6df2e656830c857c79.
The path-confinement change rejects repository-relative traversal and symlink escapes for campaign
outputs, required paths, and the declared campaign configuration; its focused symlink regression
passes. The strict decision-capable admission correction is present and fail-closed:
analysis.dry_run_status must be passed, design.power_status must be adequate, and waived
values return named blocked states rather than answerable. Required proof remains surface-specific
and bound to exact manifest, config, receipt, catalog, and packet identities; fixture, fallback,
degraded, unavailable, failed, and not-run required proof remains non-admitting. This is launch-gate
infrastructure, not a campaign or scientific result.
Local proof
- Exact current-base answerability, preflight, manifest-runner, #3425 wrapper, camera-ready, and
manifest-contract suite: 113 passed. - Ruff check/format, Python compilation, Bash syntax, and
git diff --checkpassed for the focused
changed surface. - Bounded #3425 smoke with all-extras/rvo2 available: the default preflight emitted 27
diagnostic_onlyrows and completed camera-ready preflight; the requiredRUN_CAMPAIGN=1path
stopped with exit 2 andstate=not_declaredbefore campaign execution. No benchmark summary or
compute action was produced. - Hosted validation for the remote head: 31 checks passed and 2 expected checks were skipped
(coverage-gate,merge-queue-gate); no failures or pending checks. - Source-equivalent current-base readiness on local merge commit
2b52d5803ea0b828b4cfbec42f7cb46567cae493reached4880 passed, 3 skippedbefore stopping on
a host-only CUDA/NVML initialization failure intests/test_seed_utils.py. The former #7692
assertion-inventory blocker and #7634 evidence-registry companion blocker are resolved in current
main; no clean readiness stamp is claimed. CUDA failure is environment evidence only, not a GPU
or provider result. - No campaign, simulator episode, SLURM submission, compute action, model download, evidence
admission, publication, or scientific result was produced.
Domain and merge boundary
The bounded direction approval for #7090 remains separate from implementation proof. The PR is
correctly kept draft/blocked: independent domain-aware approval, dependency/parent direction, and a
clean current-base readiness gate remain open. No merge-ready admission is appropriate.
base-policy: ordinary-cas @ cbb7470f0d081fb257b22f6df2e656830c857c79
review-claim: reviewed @ a257ff17562f7654d75131fbf8c6d604d36637a5
gate-verdict: blocked @ a257ff17562f7654d75131fbf8c6d604d36637a5; domain/dependency approval and clean readiness remain open
merge-ready: no @ a257ff17562f7654d75131fbf8c6d604d36637a5
pr-metadata: reconciled @ 1ef4a1d1a950f90026a16969f4b28e4f43ebaff397e0ce443c95e790b9449a11
## Summary Make the base-sensitive gate recover complete changed-file inventories from GitHub's paginated pull-request files API when `gh pr diff --name-only` is rejected for an oversized diff, while preserving fail-closed behavior for incomplete or malformed data. ## Linked Issues - Closes #7695 ## Stack / Dependency - Base dependency: `origin/main` `2d221f7c3af34e9ae9db4a563c576fb6e919771e` - Required prior PRs: none - Stack follow-up issues: none - Safe to review independently: yes - Review dependency reason, if any: none ## What Changed - Keep the successful `gh pr diff --name-only` path for ordinary small diffs. - Add strict, bounded pagination over `pulls/<number>/files` as the fallback source. - Emit structured JSON for unknown changed-file classification instead of returning an empty error path. - Add fast-path, oversized-diff, multi-page, malformed-response, and JSON fail-closed tests. - Document changed-file provenance in `docs/dev_guide.md`. ## Why It Matters - Added value: large research/documentation PRs no longer lose base-sensitive admission diagnostics solely because GitHub cannot render their unified diff. - Expected impact: complete inventories remain classifiable; partial or unavailable inventories still block safely. - Why this is worth merging now: PR #7469 reproduced HTTP 406 `PullRequest.diff_too_large` and currently prevents the gate from producing a useful JSON result. ## Research Result Guidance - Target claim / hypothesis / blocker this should affect: unblock reliable workflow classification for oversized research PRs; no scientific claim. - Comparator or baseline, if applicable: existing `gh pr diff --name-only` fast path. - Evidence tier: NA - Evidence applicability: NA - support tooling only; no benchmark or research result is produced. - Result classification: NA - Decision or stop rule, if applicable: merge only with focused tests, lint, exact-head review, and final readiness proof; unknown inventories remain fail-closed. - Parent issue, claim map, registry, context note, or synthesis surface to update: #7695. - New research/benchmark/metric/paper-facing analysis tool, if any: NA - support helper; it does not interpret evidence. ## Domain-Aware Approval - Required for this PR: no - support tooling only; no evidence or scientific claim changes. - Domains reviewed: NA - Status: not required - Approver/review source or waiver: not applicable. - Validity checklist. Keep these machine-detected labels unchanged: - Target claim/hypothesis: NA - support tooling only. - Comparator or split/evidence validity: NA - support tooling only. - Fallback/degraded exclusions: NA - no benchmark execution. - Claim boundary: no benchmark, metric, planner, publication, or scientific claim. - Implementation integrity vs experimental validity: implementation integrity is covered by focused tests and readiness; experimental validity is not in scope. ## Falsification / Non-Transfer Check - Did the mechanism activate? NA - support tooling only. - Did the intervention change command source, selected command, trajectory, or route progress? NA. - Did the scenario actually contain the targeted failure mode? NA - this is a GitHub API boundary. - Result route: NA - Follow-up question or issue for weak, negative, or non-transfer results: NA. ## Next Empirical Action - Rerun needed: NA - support tooling only. - Extractor or analysis tool needed: NA. - Artifact missing or unavailable: none for this support change. - Stop / revise / continue decision: continue using the gate; refresh any stale PR before merge. - Proposed child issue or existing follow-up: none. ## Validation / Proof - Commands run: - `uv run ruff format --check scripts/dev/check_base_sensitive_gates.py tests/dev/test_base_sensitive_gate_contract.py` — pass - `uv run ruff check scripts/dev/check_base_sensitive_gates.py tests/dev/test_base_sensitive_gate_contract.py` — pass - focused changed-file tests — 5 passed - related base-sensitive selector/worktree/CAS tests — 16 passed - `gh pr diff 7469 --repo ll7/robot_sf_ll7 --name-only` — expected HTTP 406 reproduced - `python scripts/dev/check_base_sensitive_gates.py --pr 7469 --repo ll7/robot_sf_ll7 --json` — structured ordinary classification from REST fallback - Evidence that the change works here: mocked oversized-diff and multi-page REST fixtures prove complete enumeration; malformed data and unknown JSON paths remain fail-closed. - Benchmarks or smoke tests, if applicable: NA - support tooling only. ## Risks / Rollout - Compatibility risks: the fallback adds bounded GitHub API reads only after the existing diff path fails; callers without a complete API response remain blocked. - Failure modes: API transport errors, malformed pages, empty inventories, and pagination exhaustion return `unknown` rather than admitting a PR. - Rollback or fallback plan: revert this commit; the original diff-only path remains available, and no merge-policy relaxation is introduced. ## Docs / Provenance - Updated docs: `docs/dev_guide.md` merge-race prevention section. - Relevant design or provenance notes: issue #7695 and the existing strict REST pagination contract in `scripts/dev/merge_queue_gate.py`. - Any assumptions that need to be preserved: only a complete, filename-validated inventory may reach the existing selector. ## Downstream Propagation - Parent issue updated (yes/no/NA): yes — #7695 contract and evidence updated. - Claim map / benchmark report updated (yes/no/NA): NA. - Leaderboard / artifact catalog updated (yes/no/NA): NA. - Registry or config index updated (yes/no/NA): NA. - Context index / memory note updated (yes/no/NA): NA. - Follow-up issue opened for deferred propagation (yes/no/NA): NA. - Not applicable because: support-only workflow diagnostics produce no research artifact. ## Follow-Up Issues - Deferred work: none. - Issues opened for follow-up: none. ## Reviewer Notes - Verify the fallback is only used after the diff fast path fails and that a full terminal page is required before classification. - Known limitations: the fallback is bounded at 100 API pages and deliberately reports unknown beyond that bound. - Shared-helper migration: inapplicable; this patch extends one existing gate owner and does not migrate a process-boundary helper or create a second merge authority.
Review claim
|
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head research-admission review — PR #7469
Reviewed exact live owner head a257ff17562f7654d75131fbf8c6d604d36637a5 against freshly
fetched origin/main 47702967c55666059c83a080e6e7a5fca2771911.
Findings
No actionable implementation correctness finding remains in the exact three-file delta. The
path-confinement repair rejects repository-relative traversal and symlink escapes for declared
output roots, required output paths, and the campaign configuration. The documentation change
removes only duplicate guidance already present in the parent commit. The new regression directly
exercises the resolver's symlink-escape rejection; the wider focused suite covers the production
answerability and camera-admission callers.
The branch is stale relative to the current base: it is 65 commits behind and 20 commits ahead of
origin/main (merge-base 75387dd4dbbaac7f23474b6785df1327ad31dc8d). The exact PR body still names
an older current-base review. A clean source-equivalent merge with current main was possible and
the focused suite passed, but no full current-main readiness stamp is claimed. Refresh the branch
and rebuild the final body/readiness evidence before any merge consideration.
Validation
- Exact-head focused suite: 113 passed serial.
- Exact-head focused suite: 113 passed with
pytest -n 2. - Source-equivalent current-main merge: 113 passed focused suite.
- Ruff check, Ruff format check, Python compilation, and
git diff --check: passed. - Hosted checks at exact head: 31 successes and 2 expected skips (
coverage-gate,
merge-queue-gate); all are terminal but from 2026-08-21. - No campaign, simulator episode, SLURM submission, compute action, model download, evidence
admission, publication, or scientific result was produced.
Disposition
The PR remains correctly draft and blocked. Required domain-aware approval for #7090 launch-gate
semantics and a fresh current-main readiness proof remain open; the closed #7692/#7634 blockers do
not by themselves establish a clean current-base readiness result. No merge-ready label or merge is
authorized.
review-claim: reviewed @ a257ff17562f7654d75131fbf8c6d604d36637a5
base-policy: ordinary-cas @ a257ff17562f7654d75131fbf8c6d604d36637a5
gate-verdict: blocked @ a257ff17562f7654d75131fbf8c6d604d36637a5; stale base, domain approval, and fresh readiness remain open
merge-ready: no @ a257ff17562f7654d75131fbf8c6d604d36637a5
Readiness-blocker refresh — 2026-08-28The local infrastructure blocker named in the PR body has changed. #7972 is closed, and current This does not establish current-head readiness for this PR. The PR is still draft and its #7090 lane remains Disposition remains |
1 participant
Summary
Refs #7090, #7031, and #7029.
Wire the existing
research_answerability.v1contract into the canonical camera-ready campaignadmission owner while retaining the frozen-snapshot research-yield dimensions. The implementation
is fail-closed and diagnostic: it produces no campaign, evidence artifact, or scientific result.
What changed
analysis, artifact, and result-packet proof surfaces through existing owners.
estimand, receipt, catalog, and result-packet identities and selected artifact digest sets.
symlink targets, and return structured failures for malformed receipt identities.
--research-manifest --require-answerableto the camera-ready launcher and routed theresearch: run one SLURM-to-claim vertical slice through finalizer evidence #3425 wrapper through the same gate on preflight and run.
unavailable, failed, and not-run required proof cannot authorize admission.
Research and evidence boundary
or benchmark claim is made.
no experimental split or scientific comparator is introduced.
Exact refresh
cbb7470f0d081fb257b22f6df2e656830c857c79(origin/main).a257ff17562f7654d75131fbf8c6d604d36637a5./home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260821l.commit
2b52d5803ea0b828b4cfbec42f7cb46567cae493for readiness proof; the owner branch remainsunchanged and no source push was attempted.
Domain-Aware Approval
merge-readyuntil domain-aware approval is recorded.Validity checklist
proof to enter its decision-capable admission path; this PR does not establish a scientific result.
evidence-contract, analysis, artifact, and result-packet surface; no experimental comparator is
introduced or pooled.
controlled-diagnostic proof remain non-admitting for required decision-capable surfaces.
planner, campaign, ranking, publication, dissertation, or model-wide claim.
fail-closed implementation contract; domain review separately decides whether the admission
semantics are valid for research use.
Validation / proof
contract suite: 113 passed for the source-equivalent current-base review tree.
a257ff17562f7654d75131fbf8c6d604d36637a5: 31 checks passed and2 expected checks were skipped (
coverage-gate,merge-queue-gate); no failures or pending checks.4880 passed, 3 skippedbefore stopping on ahost-only CUDA/NVML initialization failure in
tests/test_seed_utils.py. The former friction: refresh production assertion inventory after map-runner change #7692assertion-inventory blocker and friction: refresh evidence registry baseline after #7410 receipt #7634 evidence-registry companion blocker are resolved in current
main. No clean readiness stamp was produced; changed-line coverage and downstream readiness claims
remain unestablished. CUDA failure is environment evidence only and is not a provider or GPU result.
27diagnostic-only rows and completed camera-readypreflight with
rvo2available;RUN_CAMPAIGN=1exited2atnot_declaredanswerability beforecampaign execution. No episodes, compute, evidence admission, or scientific result was produced.
git diff --checkpassed for the focusedchanged surface.
Risks / rollout
Required callers opt into the gate with both
--research-manifestand--require-answerable; thereadiness-only path remains available without it. Malformed manifests and required proof failures
return structured blocked output. The #3425 wrapper re-evaluates exact manifest/config binding after
preflight. Rollback is a revert of the admission-hook changes.
Docs / downstream propagation
docs/benchmark_campaign_manifest.mdanddocs/context/issue_3425_slurm_to_claim_blocker.md.Reviewer notes
Verify canonical proof identities, pre/post digest binding, required versus optional proof handling,
fixture rejection, and fail-closed launch behavior. Domain interpretation remains separate from
implementation/schema checks.
Disposition
gate-verdict: blocked @ a257ff17562f7654d75131fbf8c6d604d36637a5; domain/dependency approval and a clean current-base readiness gate remain openmerge-ready: no (Domain-Aware Approval remains pending; current-base readiness has a host-only CUDA/NVML blocker and no clean stamp)PR Metadata
a257ff17562f7654d75131fbf8c6d604d36637a5cbb7470f0d081fb257b22f6df2e656830c857c79/home/luttkule/git/robot_sf_ll7.worktrees/pr-7469-review-current-20260821l